fix(runtime-host): fence State Root before service retirement - #3602

Pull request overview

This PR hardens the managed Runtime Host “PID-known” retirement flow so it won’t stop a successor host (and misreport retirement) when systemd restarts the service unit between host.upgrade.prepare and backend.stop(). It does this by waiting to fence the State Root before issuing the service stop, and by failing closed when the observed service identity changes.

Changes:

• Acquire and hold a State Root retirement fence (with PID re-checking) before calling backend.stop() in the PID-known retirement path.
• Convert the fence acquisition into a bounded polling loop with an explicit deadline and clearer failure modes.
• Add regression tests for successor replacement during the drain window and for “fence held across stop” behavior.

Required Conclusion (per review skill)

1. Is the current solution optimal for the actual problem?
   Mostly, but there is a remaining race where a PID change may not be detected if it happens right as the fence becomes available; this can still allow stopping a successor under some timing conditions (see stored comment).
2. What production code can be deleted?
   none identified.
3. What low-quality tests can be deleted or replaced?
   none identified.
4. Is a deeper refactor required, and what should the final structure be?
   No deeper refactor is required for this fix; tightening the fence/PID verification logic inside acquireRuntimeHostRootRetirementFence should be sufficient.
5. Is the reviewed revision ready to merge?
   No.
6. What residual risks or verification gaps remain?
   The safety property hinges on correctly treating PID changes as fatal across the entire fence acquisition boundary; the current implementation appears to miss a narrow race at the “fence becomes available” transition. Additionally, the PR notes Linux/systemd runtime verification has not been performed yet.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Reviewed at exact head 52357a78fe70ab272a1ed231d49eb4f445cabc2c (2 commits, 2 files, +305/-4: 257 lines of tests and ~48 lines of production code). No [P0]–[P2] findings — the fence holds from all four directions I checked. Notes below, plus one bounded observation and a CI caveat.

What the fence actually fences

The fence is the existing interactive-writer lock on the State Root, held from a successful prepare until the service confirms stopped. That matters because it means there is no second state machine: the PR reuses the one existing authority over root writes rather than inventing a parallel notion of "retiring".

The ordering is what makes it sound. prepareRetirement cross-checks diagnostics.pid against expectedPid, then host.upgrade.prepare(expectedHostEpoch) puts the host into drain, and #requestDrain → #closeResources stops new admissions, waits for in-flight operations, closes composition, waits for residency, and only then calls owner.close() to release the writer. So by the time the CLI's poll loop acquires the lock, in-flight work has already landed. The lock acquisition is itself the drain proof, not a separate signal that could disagree with reality.

Ordering and atomicity

The window between fencing and backend.stop() is guarded twice: isExpectedRuntimeHostRetirementStatus runs on every poll iteration and once more immediately after the lock is acquired. A successor process is rejected because its pid differs; the only other accepted state is a fully stopped host (pid === null && !active && state === 'stopped'), which cannot be a live successor.

CLI crash between the two steps is recoverable rather than a half-retired hang: the fd lock dies with the process, and the host's shutdown is self-driven from the drain, not dependent on the CLI issuing the stop.

Failure direction

Every failure mode I traced — prepare failure, connect failure, pid mismatch, fence timeout, identity change — throws retirement_failed before backend.stop(). The new tests pin this with stops === 0, which is the right assertion: it proves the stop never happened, not merely that an error surfaced. The one path that throws after stop() is the post-stop stable-state check, where retirement has already taken effect; that is a loud failure rather than a fail-open, and a retry converges through the idempotent 'stopped' branch.

[P3] Point-in-time recheck leaves a small unguarded gap

Between the final identity recheck and backend.stop() there is no continuous guard. If the old host dies in that gap and systemd starts a successor, the successor is locked out of the writer by the fence the CLI still holds, then gets hit by the stop — and the returned retired result still reports the old pid and hostEpoch.

The consequence is bounded (no writes occur, the root ends up idle), and converting a point check into continuous monitoring is not worth the cost here. Recording it for the record rather than asking for a change.

Validation and CI

Red-green run locally: reverting the production change while keeping the new tests turns exactly the 3 new tests red; restoring the PR gives 19/19 green. Typecheck clean.

CI cannot be called green at this head. This is a fork PR, check-runs is empty, and the only workflow run (32636922940) is action_required — never approved to execute. That is a fork permission gate rather than anything about the code, but it means there is no hosted exact-head evidence, and a maintainer should approve the workflow run before this is treated as gated.

Approving at exact head 52357a78fe70ab272a1ed231d49eb4f445cabc2c.

Hosted CI has now run on this head — the earlier action_required state was a fork permission gate, and with the run executed, test is completed/success. That closes the one thing my previous comment held open. The findings position is unchanged: no [P0]–[P2], and the single [P3] (the unguarded gap between the final identity re-check and backend.stop()) is recorded rather than requested.

One sequencing note for whoever merges. #3591 touches this same file and carries a different, weaker version of acquireRuntimeHostRootRetirementFence — the single-attempt form, not wired into the retire path at all. Landing this PR first makes that resolution additive; landing #3591 first risks the polling-plus-identity-recheck hardening being silently dropped when the conflict is resolved. Merging this one ahead of #3591 is the safer order.